Skip to content

Feature: PDF support - #78

Open
nicholas477 wants to merge 21 commits into
liamw1:masterfrom
nicholas477:feature/pdfs
Open

Feature: PDF support#78
nicholas477 wants to merge 21 commits into
liamw1:masterfrom
nicholas477:feature/pdfs

Conversation

@nicholas477

Copy link
Copy Markdown

Not really sure if this would be wanted, but I added PDF support to my fork and I was curious if y'all would want this upstream

liamw1
liamw1 previously requested changes Aug 9, 2026

@liamw1 liamw1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cool! I'm totally willing to support additional formats as long as they meet two conditions:

  1. It's a format that is supported by all the major browsers
  2. There's some way to extract an image that represents its contents

Seems like PDFs satisfy both, so they're a good candidate.

I reviewed the implementation: the structure seems fine, just needs a bit more massaging to get it consistent with the rest of the codebase. For the comments about adding ApiError variants: wiring those up can be a bit tedious, so if you want I can handle those changes.

I have one comment on the design overall. What do you think of using max_image_width and max_image_height for the max dimensions of the PDF instead of having separate config values just for PDFs? I say this because no other post type has separate dimension limits, so I feel like it may make sense to merge them.

Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
Comment thread server/src/model/enums.rs Outdated
Comment thread server/src/content/pdf.rs Outdated
@nicholas477

nicholas477 commented Aug 9, 2026

Copy link
Copy Markdown
Author

Thank you for taking the time to look over my code! I'm going to implement all of the code changes in a bit. About the image size though:

I have one comment on the design overall. What do you think of using max_image_width and max_image_height for the max dimensions of the PDF instead of having separate config values just for PDFs? I say this because no other post type has separate dimension limits, so I feel like it may make sense to merge them.

I don't think that would make sense since PDFs don't really have a pixel width/height. I'm not too sure how the library calculates the size, but for some of the PDFs i was giving it, it would give me somewhere in the range of 16k image width/height for the first page.

Also the max_pdf_width/height is only used for:

  • limiting the size of the preview image shown in the posts list
  • limiting the size of the PDF preview inside the post page

I think there's a better way to do both. I'd like to uncouple the size of the pdf viewer from the size of the preview image. Maybe the preview image should be around 300px? I didn't look into how oxibooru calculates size of the preview images for pngs, jpgs, etc. For the size of the pdf shown on the page, I'm not really sure how that should be done right. Here's an example of how this looks right now:
image

I'm not much of a frontend dev unfortunately, but scaling it to page seems like the way

@liamw1

liamw1 commented Aug 9, 2026

Copy link
Copy Markdown
Owner

I'm not too sure how the library calculates the size, but for some of the PDFs i was giving it, it would give me somewhere in the range of 16k image width/height for the first page.

So if you were to use default render settings:

// Defaults to x/y scale of 1.0 and library chooses width/height
RenderSettings {
    bg_color: WHITE,
    ..Default::default()
}

the library would try to render a huge preview? These settings seem to work OK for the PDFs I've uploaded, but there might be some weird edge cases I haven't run into.

@nicholas477

nicholas477 commented Aug 9, 2026

Copy link
Copy Markdown
Author

I'm not too sure how the library calculates the size, but for some of the PDFs i was giving it, it would give me somewhere in the range of 16k image width/height for the first page.

So if you were to use default render settings:

// Defaults to x/y scale of 1.0 and library chooses width/height
RenderSettings {
    bg_color: WHITE,
    ..Default::default()
}

the library would try to render a huge preview? These settings seem to work OK for the PDFs I've uploaded, but there might be some weird edge cases I haven't run into.

Yeah, I've been making PDFs for sewing patterns (exported from clo3d) and the calculated dimensions seem to be absolutely massive. for this pdf: https://booru.cactus.vg/data/posts/000000/00/1040_GdEGVuUVFyo6s6yh9Kasbzse3uI09CaKsA5P3EViUd8.pdf it was giving me 7746 x 9804 for the size

also once again, i think the rendered image is only used for the preview thumbnail in the post list. i dont show it in the post viewer itself at least

@liamw1

liamw1 commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Alright, in that case it seems like the separate dimension limits for PDFs defensible. I'm only harping on this because taking away settings from the config is more painful than adding them, so I want to be confident they're necessary.

@nicholas477

Copy link
Copy Markdown
Author

I feel that! If there is a better way to do any of this please let me know, I'm also not really happy with adding more config variables

nicholas477 and others added 3 commits August 9, 2026 20:26
Renamed ratio to scale for PdfRenderDimensions
Moved `PdfLoadError` to `api::error.rs` and made `LoadPdfError` convertible to `ApiError`.

Moved private implementation details in `pdf.rs` to bottom.

Made `from_page` fallible instead of `render_settings`.

Trimmed some spacing and intermediate variables.
@liamw1

liamw1 commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Alright, I went ahead and committed some minor stylistic changes that weren't worth commenting on here. I also removed the need for cloning the pixel buffer with a bit of unsafe code.

Everything looks pretty good to me. I'm aiming to get this into version 0.9. Until then, I'll keep thinking about alternative ways of handling the preview scaling and see if we can remove the need for those two additional config settings.

Something else that's worth thinking about is up-scaling. One PDF that I uploaded while testing came out to have a fairly small preview image (around 200px in width/height). Because the preview dimensions are used to determine the size of the post view, the PDF is rendered in a very small area when viewing the post. Perhaps the preview image should be scaled to some minimum size as well?

@nicholas477

Copy link
Copy Markdown
Author

Looks great! Thanks for cleaning up the code. I agree we also need a minimum size and I can code that up, just where do I figure out what the minimum size needs to be?

@nicholas477

Copy link
Copy Markdown
Author

Also is there a discord or something for this project?

PDF posts now respect rescale options. Since the "size" of the PDF is somewhat arbitrary, small PDFs are always upscaled regardless of the value of `upscaleSmallPosts`.
@liamw1

liamw1 commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Also is there a discord or something for this project?

No, not at the moment

Also, I think most of these scaling issues can be solved by the client. The post content template was silently broken and so PDFs did not respect scaling buttons. I fixed that and also made it so that PDFs will small preview images are always scaled up on the client, which looks much better IMO. The dimensions of a PDF are somewhat arbitrary anyway.

So instead of having a minimum size for the preview, the client will automatically enlarge PDFs that happened to have small previews for whatever reason. And after thinking about it for a bit, I think it actually makes sense to make the max preview dimension hard-coded constants. The dimension limiting is mostly an implementation detail now, since the client is handling the scaling.

@liamw1
liamw1 dismissed their stale review August 10, 2026 03:17

Resolved.

@nicholas477

Copy link
Copy Markdown
Author

I think the fit width/height buttons should work differently for documents. I don't see why it should keep the aspect ratio the same. I think i'm going to try changing how this works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants